Format all numbers with BigNumber #333
Conversation
Deployed to Cloudflare Pages
|
csillag
force-pushed
the
csillag/number-formatting
branch
3 times, most recently
from
71769f8 to
9be2492
Compare
csillag
force-pushed
the
csillag/number-formatting
branch
3 times, most recently
from
6273034 to
303e647
Compare
src/app/utils/numberFormatter.ts
Outdated
| const { decimalPlaces, maximumFractionDigits, roundingMode, unit, ...formatting } = format | ||
| let number = | ||
| typeof inputNumber === 'number' | ||
| ? BigNumber(inputNumber.toString(2), 2) // This is required to keep all precision |
There was a problem hiding this comment.
I don't believe this comment. How
There was a problem hiding this comment.
If the limited precision of Number values is not well understood, it is recommended to create BigNumbers from String values rather than Number values to avoid a potential loss of precision.
When creating a BigNumber from a Number, note that a BigNumber is created from a Number's decimal
toString()value not from its underlying binary value. If the latter is required, then pass the Number'stoString(2)value and specify base 2.
There was a problem hiding this comment.
So, to summarize: according to the docs, in order to get all the data from a Number, with no loss of precision, we need to create a binary string representation. Otherwise it will just create a string (with whatever precision) and that will be the input of BigNumber.
There was a problem hiding this comment.
okay it loses precision differently
BigNumber((10000000000/3).toString(2), 2).toFixed()
'3333333333.33333349227905273438'
BigNumber(10000000000/3).toFixed()
'3333333333.3333335'
|
Finished reacting to the first round of comments. Ready for second round. |
|
Fixing the broken test case... |
src/app/hooks/useNumberFormatter.ts
Outdated
| if (number.toNumber() === 1) { | ||
| return t(countKey as any, { count: 1 }) | ||
| } else { | ||
| return t(countKey as any, { count: 2 }).replace('2', numberString) | ||
| } |
There was a problem hiding this comment.
OK I have a new solution now, which should take care of it.
There was a problem hiding this comment.
The new solution is
const i18nForm = t('common.number', { value: num })
return t(countKey as any, { count: num }).replace(i18nForm, numberString)I've been thinking if there are any solutions that don't involve string manipulation, and I have one:
return t(countKey as any, {
count: number.toNumber(), // To pluralize the translation strings
value: numberString, // To display in translation strings (without losing precision, and with consistent formatting)
})
| ? BigNumber(inputNumber.toString(2), 2) // This is required to keep all precision | ||
| : BigNumber(inputNumber) | ||
| if (maximumFractionDigits !== undefined) { | ||
| number = BigNumber(number.toFixed(maximumFractionDigits, roundingMode)) |
There was a problem hiding this comment.
how come not number.decimalPlaces
| const numberString = | ||
| decimalPlaces === undefined | ||
| ? number.toFormat(wantedFormat) | ||
| : roundingMode === undefined | ||
| ? number.toFormat(decimalPlaces, wantedFormat) | ||
| : number.toFormat(decimalPlaces, roundingMode, wantedFormat) |
There was a problem hiding this comment.
If bignumber types weren't broken I would modify to:
return (
inputNumber: number | string | BigNumber.Instance | undefined,
format: NumberFormattingParameters = { roundingMode: BigNumber.ROUND_DOWN },
): string | undefined => {
const numberString = number.toFormat(decimalPlaces, roundingMode, wantedFormat)
csillag
force-pushed
the
csillag/number-formatting
branch
from
7bb1b6b to
84a2c50
Compare
| if (num === 1) { | ||
| return t(countKey as any, { count: 1 }) | ||
| } else { | ||
| const i18nForm = t('common.number', { value: num }) |
There was a problem hiding this comment.
#731 (comment)
We might be able to simplify a lot
Not handled: